Cache and print devices for debugging future outages #2141

cemakd · 2025-08-04T18:05:43Z

This is a copy of PR: #2097

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR adds a cache that periodically (configured to every minute currently) looks at the /dev/disk/by-id/ directory and evaluates the symlinks there. It maintains a cache of the symlink and the real path it points to.

This will help with debugging future filesystem issues. In a past OMG, we found that our insight into changes in symlinks for specific disks hampered our ability to debug. Logging marked the real path of the disk at mount and unmount, but the change in between couldn't be detected.

This PR will print those links every minute, also logging when elements of the cache change.

An example:

I0521 00:23:51.418261      12 cache.go:62] periodic symlink cache read: /dev/disk/by-id/google-persistent-disk-0 -> /dev/sda; /dev/disk/by-id/google-pvc-f5418f78-dc07-4d69-9487-6c4a7232dd67 -> /dev/sdb; /dev/disk/by-id/scsi-0Google_PersistentDisk_persistent-disk-0 -> /dev/sda; /dev/disk/by-id/scsi-0Google_PersistentDisk_pvc-f5418f78-dc07-4d69-9487-6c4a7232dd67 -> /dev/sdb

The cache will also note if a symlink is broken.

NOTE: Currently this filters out any thing in by-id/ that ends with -part[0-9]*$. This removes partitions, which are noise. Mounting partitions directly isn't well supported in GKE, but we may want to test that in the future.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

changes and printing the full list. example: periodic symlink cache read: /dev/disk/by-id/google-persistent-disk-0 -> /dev/sda; /dev/disk/by-id/google-pvc-f5418f78-dc07-4d69-9487-6c4a7232dd67 -> /dev/sdb; /dev/disk/by-id/scsi-0Google_PersistentDisk_persistent-disk-0 -> /dev/sda; /dev/disk/by-id/scsi-0Google_PersistentDisk_pvc-f5418f78-dc07-4d69-9487-6c4a7232dd67 -> /dev/sdb

easier to unit test.

right now.

k8s-ci-robot · 2025-08-04T18:05:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cemakd
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tonyzhc · 2025-08-04T18:13:34Z

/ok-to-test

msau42

Can you describe the testing that has been done for this change?

msau42 · 2025-08-05T23:58:33Z

pkg/linkcache/devices_linux.go

+	return newDeviceCacheForNode(period, node), nil
+}
+
+func TestDeviceCache(period time.Duration, node *v1.Node) *DeviceCache {


Can we move the test functions to the _test.go file?

These were not real test function but rather creating a DeviceCache for testing purposes used by other packages. I've renamed them to NewTestDeviceCache to distinguish them from actual test functions.

msau42 · 2025-08-06T00:01:49Z

pkg/linkcache/devices_linux.go

+	// of the string (after the last "/") and call AddVolume for that
+	for _, volume := range node.Status.VolumesInUse {
+		klog.Infof("Adding volume %s to cache", string(volume))
+		vID, err := pvNameFromVolumeID(string(volume))


Does this ignore volume types that are not PD/hyperdisk?

It was not ignoring them, I've pushed a commit that checks if the volume is for the driver pd.csi.storage.gke.io. IIUC that should be sufficient to do the filtering.

msau42 · 2025-08-06T00:03:07Z

pkg/linkcache/devices_linux.go

+		// Evaluate the symlink
+		realPath, err := filepath.EvalSymlinks(device.symlink)
+		if err != nil {
+			klog.Warningf("Error evaluating symlink for volume %s: %v", volumeID, err)


Be careful of logging this since this is called repeatedly.

Now that the polling period is 10 mins, would it be okay to log this? Otherwise it might be hard to figure out any problems with realpath evaluation?
I'm not sure if that's a realistic problem to log, I can remove this warning log if you think it's not necessary

msau42 · 2025-08-06T00:06:24Z

cmd/gce-pd-csi-driver/main.go

@@ -275,6 +276,13 @@ func handle() {
 			klog.Fatalf("Failed to get node info from API server: %v", err.Error())
 		}

+		deviceCache, err := linkcache.NewDeviceCacheForNode(ctx, 1*time.Minute, *nodeName)


Can we make the time period configurable so that we can adjust it if it ends up spamming too many logs? In fact I would probably start the logging at 10 mins initially.

In the future, if we want to turn this into a metric, I would be more comfortable reducing the polling period but removing logs to avoid log spam.

Done, added a flag disk-cache-sync-period with a default value of 10 minutes when unspecified.

msau42 · 2025-08-06T00:08:46Z

pkg/linkcache/devices_linux.go

+	}
+
+	// Look at the dir for a symlink that matches the pvName
+	symlink := filepath.Join(d.dir, "google-"+deviceName)


Has this been tested with nvme disks as well? It would be better to reuse functions in https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/pkg/deviceutils/device-utils.go

I haven't done specific testing for nvme disks but I've pushed a new commit that utilizes device-utils.go for evaluating the symlinks for a volumeID

cemakd · 2025-08-07T17:34:19Z

Can you describe the testing that has been done for this change?

I haven't done any additional testing on this PR since I took it over from Julian. As far as I know he was struggling to get the e2e tests passing, which I fixed with a few nil pointers checks.

I'll do some more testing on this and get back to you

Change volumes map to use symlink as key and add volumeID and real path to the deviceMappings object, this is because there can be multiple symlinks per volume Filter out caching and logging info on non pd/hyperdisk volumes Failure to get real path at add volume time is no longer an error the real path may be unavailable at this time Fix the device_windows.go NewDeviceCacheForNode parameters

k8s-ci-robot · 2025-08-07T21:58:14Z

@cemakd: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gcp-compute-persistent-disk-csi-driver-e2e-arm	`61aeffd`	link	false	`/test pull-gcp-compute-persistent-disk-csi-driver-e2e-arm`
pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration	`61aeffd`	link	true	`/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

julianKatz added 21 commits August 1, 2025 17:34

Some doc comment updates

d3a824e

Add unit tests

485beaf

improve partition unit test

afb2393

Log on removal as well

643817f

Updated unit tests to be clearer, relying on asserting linkCache

fa2d2f9

Remove unused broken function

95163b7

Move partition checking into the inner linkcache type. This makes it

8d8d926

easier to unit test.

Log when linkcache Run is triggered

448b0ba

New implementation that is hooked into nodestage/unstage. Just linux

2ef351c

right now.

Made a no-op windows implementation of the linkcache package

9ab0d2f

Made test device caches in node_test.go

b88e5f4

Fix sanity test

d76c44c

Only warn on failure to create cache

4abd540

Only warn on windows instantiation

c4b69f3

Make non-implemented on windows an info

042176a

Improved some error messages to provide better test failure feedback

bc8defa

Always print helpful logs in failing area

170e24b

Remove now unnecessary corp-helper when running from cloudtop

be4b045

Only run device cache if successfully created

6ef07a2

Replace verbosities

3437efd

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 4, 2025

k8s-ci-robot requested a review from julianKatz August 4, 2025 18:05

k8s-ci-robot requested a review from tonyzhc August 4, 2025 18:05

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 4, 2025

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 4, 2025

Add nil checks around the usage of the device cache

3824cbb

msau42 reviewed Aug 6, 2025

View reviewed changes

cemakd force-pushed the logs-for-device-mappings branch from 61ea81b to 61aeffd Compare August 7, 2025 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cache and print devices for debugging future outages #2141

Cache and print devices for debugging future outages #2141

cemakd commented Aug 4, 2025

Uh oh!

k8s-ci-robot commented Aug 4, 2025

Uh oh!

tonyzhc commented Aug 4, 2025

Uh oh!

msau42 left a comment

Uh oh!

msau42 Aug 5, 2025

Uh oh!

cemakd Aug 7, 2025

Uh oh!

msau42 Aug 6, 2025

Uh oh!

cemakd Aug 7, 2025

Uh oh!

msau42 Aug 6, 2025

Uh oh!

cemakd Aug 7, 2025

Uh oh!

msau42 Aug 6, 2025

Uh oh!

cemakd Aug 7, 2025

Uh oh!

msau42 Aug 6, 2025

Uh oh!

cemakd Aug 7, 2025

Uh oh!

cemakd commented Aug 7, 2025

Uh oh!

k8s-ci-robot commented Aug 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

Cache and print devices for debugging future outages #2141

Are you sure you want to change the base?

Cache and print devices for debugging future outages #2141

Conversation

cemakd commented Aug 4, 2025

Uh oh!

k8s-ci-robot commented Aug 4, 2025

Uh oh!

tonyzhc commented Aug 4, 2025

Uh oh!

msau42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cemakd commented Aug 7, 2025

Uh oh!

k8s-ci-robot commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 7, 2025 •

edited

Loading